add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446

dprunier · 2024-05-29T20:45:38Z

What

Currently, there are a few command line arguments for cluster create and cluster delete that cannot be specified using environment variables. This PR tries to address this by leveraging viper, which is already being used by k3d.

Why

I'd like to set a default configuration for k3d, and the easiest way would be to have K3D_CONFIG=${HOME}/.k3d/config.yaml in my shell environment.

Another approach could have been to use viper configuration loading mechanism (which has a mechanism to locate configuration files from various locations) but it would have been a more involving change.

Implications

I don't think there are any, as the change is relatively minor and backward compatible.

iwilltry42 · 2024-07-04T09:29:34Z

Hey @dprunier , thanks for this PR!

For some reason, I have no way of triggering the test suite on your PR - but it seems to fail on my machine.
Can you please give it a try?
It's good enough to only run the simple config file test: make e2e -e E2E_FAIL_FAST=1 -e E2E_LOG_LEVEL=debug -e E2E_INCLUDE="test_config_file"

…variables

iwilltry42

The config test is still failing for me - it seems like it doesn't consider the config passed in via --config at all anymore

EDIT: Since you removed the var, where we stored the --config flag value before, we don't have access to the value anymore as ppViper doesn't know about that flag. Here's a patch (renamed as txt to allow that in GitHub) that would solve this for clusterCreate.

dprunier · 2024-07-15T13:27:47Z

Hey @iwilltry42. sorry for the long delay, I was out for a while. Started to look at this today, it looks like the patch you attached is exactly the current diff, am I missing something ? Thx.

iwilltry42 · 2024-07-15T14:44:04Z

@dprunier whoops, yeah you're right, I attached the wrong patch. Here's the correct one:
0001-fix-config.patch.txt

dprunier · 2024-07-15T15:39:54Z

@iwilltry42 nevermind, I think I found the issue... it was a stupid oversight on my end (and clearly lack of proper testing :D)

The test is now passing but raised a concern with the behavior of the delete command: currently, when a configuration file is passed, it is assumed that it must contain a cluster name (as this is the only real reason to pass a config file in the first place).

The problem is that in my case, I have a system wide K3D_CONFIG environment variable set (to set a few defaults for my own use) but it doesn't contain a name. In such case, the delete command never works and always fails with failed to delete cluster via config file: no name in config file.

I believe there would be an easy fix: when a configuration file is given and contains a name, it is used, otherwise it behaves as if no configuration file was given. I can do this change real quick but:

would you be ok with this change of behavior ?
if so, should it be in that PR or another one ?

iwilltry42 · 2024-07-15T19:05:45Z

I'm fine with that slight change in behavior, which means that if a) config file does not contain a name and b) --name is unset, we default to the default cluster name k3s-default, right?

Feel free to add it to this PR and I'll account for it in the changelog and release notes 👍

…provide a name

dprunier · 2024-07-15T21:13:54Z

Cool. I've added it with fff51e9. The behavior is pretty simple: instead of failing as soon as one specifies a configuration file with --config along with some names or --all as it used to, it is only doing so if there is a name in the configuration file. If there isn't any name in the configuration file, it is just ignored (as if none was passed with --config). That's probably the closest from the existing behavior, let me know if it works for you.

With that change, the E2E test test_config_file now passes.

iwilltry42 · 2024-07-16T08:35:11Z

it is only doing so if there is a name in the configuration file

I think that --name and --all should just always take precedence over whatever there is in the config file.
I.e. a system-wide default config file that you have for example, could provide the default cluster name (like overriding k3s-default) - that would not work with the latest change, right?

dprunier · 2024-07-16T13:00:47Z

that would not work with the latest change, right?

Indeed. To make things clear, given the cluster delete syntax:

k3d cluster delete [--config FILE] [NAME [NAME ...] |--all] (Note: since this PR configuration file is specified using either --config or $K3D_CONFIG)

The current (main) behavior is:

	neither `NAME` nor `--all` specified	`NAME` specified	`--all` specified	both `NAME` and `--all` specified
no configuration	`k3s-default`	`NAME`	existing cluster names	existing cluster names, `NAME` ignored
with configuration file, and it does not contain a name	fail	fail	fail	fail
with configuration file and it contains a name	name from configuration file	fail	fail	fail

With the implementation from this PR, it looks like this (the difference is the second line):

	neither `NAME` nor `--all` specified	`NAME` specified	`--all` specified	both `NAME` and `--all` specified
no configuration	`k3s-default`	`NAME`	existing cluster names	existing cluster names _{(NAME ignored)}
with configuration file, and it does not contain a name	`k3s-default`	`NAME`	existing cluster names	existing cluster names _{(NAME ignored)}
with configuration file and it contains a name	name from configuration file	fail	fail	fail

And I believe what you're suggesting is this (the difference is the third line):

	neither `NAME` nor `--all` specified	`NAME` specified	`--all` specified	both `NAME` and `--all` specified
no configuration	`k3s-default`	`NAME`	existing cluster names	existing cluster names _{(NAME ignored)}
with configuration file, and it does not contain a name	`k3s-default`	`NAME`	existing cluster names	existing cluster names _{(NAME ignored)}
with configuration file and it contains a name	name from configuration file	`NAME` _{(name from configuration file ignored)}	existing cluster names _{(name from configuration file ignored)}	existing cluster names _{(NAME and name from configuration file ignored)}

I can do this change if you agree. It is worth mentioning that it would make it more consistent with cluster create, which indeed overrides the name from the configuration file if it is specified on the command line.

Let me know.

dprunier · 2024-09-23T18:51:26Z

@iwilltry42, any update on the above ? Thanks.

iwilltry42 · 2024-11-03T07:31:03Z

I'm really sorry @dprunier I just got to look at this.
You're absolutely right and table 3 is what I was aiming for 👍

dprunier · 2024-11-18T20:12:52Z

@iwilltry42, no worries, can't say I've been very quick either :) I've checked in the changes to implement the behavior of table 3.

The test suite seems to pass.

Let me know what you think.

iwilltry42

Yep, looks good!
Thanks a lot for this :)

iwilltry42 force-pushed the config_file_from_env branch 2 times, most recently from cfd98a3 to bef7bf8 Compare July 4, 2024 09:22

iwilltry42 self-requested a review July 4, 2024 09:29

allow config and pre-processed configuration to leverage environment …

5f74fc0

…variables

dprunier force-pushed the config_file_from_env branch from bef7bf8 to 5f74fc0 Compare July 11, 2024 20:25

iwilltry42 changed the title ~~Allow config and pre-processed configuration to leverage environment variables~~ feat: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) Jul 12, 2024

iwilltry42 changed the title ~~feat: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var)~~ add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) Jul 12, 2024

iwilltry42 approved these changes Jul 12, 2024

View reviewed changes

iwilltry42 self-requested a review July 12, 2024 09:58

iwilltry42 requested changes Jul 12, 2024

View reviewed changes

cluster delete command was not using the correct viper instance...

a824179

dprunier force-pushed the config_file_from_env branch from 6f3de1c to a824179 Compare July 15, 2024 14:59

cluster delete command was not using the correct viper instance again...

376692e

Merge remote-tracking branch 'upstream/main' into config_file_from_env

c50308f

dprunier requested a review from iwilltry42 July 15, 2024 15:46

ignore configuration provided to cluster delete command if it doesnt …

fff51e9

…provide a name

dprunier force-pushed the config_file_from_env branch from de42f1d to fff51e9 Compare July 15, 2024 21:09

Merge remote-tracking branch 'upstream/main' into config_file_from_env

8248a7c

Merge remote-tracking branch 'upstream/main' into config_file_from_env

00692d3

Merge remote-tracking branch 'upstream/main' into config_file_from_env

6087e5d

allow --all or args to override configuration name

a8445fc

iwilltry42 approved these changes Nov 26, 2024

View reviewed changes

iwilltry42 merged commit c91851a into k3d-io:main Nov 26, 2024

BrewTestBot mentioned this pull request Nov 26, 2024

k3d 5.7.5 Homebrew/homebrew-core#199106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446

add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446

dprunier commented May 29, 2024

iwilltry42 commented Jul 4, 2024

iwilltry42 left a comment •

edited

Loading

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 15, 2024

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 15, 2024

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 16, 2024

dprunier commented Jul 16, 2024

dprunier commented Sep 23, 2024

iwilltry42 commented Nov 3, 2024 •

edited

Loading

dprunier commented Nov 18, 2024

iwilltry42 left a comment

add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446

add: Allow config and pre-processed configuration to leverage environment variables ($K3D_CONFIG env var) #1446

Conversation

dprunier commented May 29, 2024

What

Why

Implications

iwilltry42 commented Jul 4, 2024

iwilltry42 left a comment • edited Loading

Choose a reason for hiding this comment

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 15, 2024

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 15, 2024

dprunier commented Jul 15, 2024

iwilltry42 commented Jul 16, 2024

dprunier commented Jul 16, 2024

dprunier commented Sep 23, 2024

iwilltry42 commented Nov 3, 2024 • edited Loading

dprunier commented Nov 18, 2024

iwilltry42 left a comment

Choose a reason for hiding this comment

iwilltry42 left a comment •

edited

Loading

iwilltry42 commented Nov 3, 2024 •

edited

Loading